CP-48676: Reuse pool sessions on slave logins #6258
Conversation
snwoods
force-pushed
the
private/stevenwo/CA-400339
branch
from
81c5fd5 to
f17fb1b
Compare
snwoods
force-pushed
the
private/stevenwo/CA-400339
branch
from
f17fb1b to
b94335a
Compare
|
The Ring3: BVT + BST suiteruns I have run were all green, both with (211574) and without (211571) additional debug logging. The debug suiterun shows that the pool sessions are being reused as expected. |
snwoods
force-pushed
the
private/stevenwo/CA-400339
branch
from
b94335a to
3efb3bb
Compare
snwoods
force-pushed
the
private/stevenwo/CA-400339
branch
from
3efb3bb to
151686b
Compare
I've marked all the comments as resolved where appropriate. This is still an outstanding question for me though. |
snwoods
force-pushed
the
private/stevenwo/CA-400339
branch
from
151686b to
c6da552
Compare
|
This is still being tested, apparently the member is not reusing the session enough times (we were trying to determine whether it is able to cope with a coordinator restart, and it seems it can, but then it is not trying to reuse the session either). Putting back into draft until we figure out whether once it is reusing the session properly if it still works. |
|
OK looked at this in more detail, and the pool session was mostly actually created on the coordinator, so this is fine (it avoids lots of DB calls to create sessions, and reuses them). |
|
Yes as Edwin said, we were initially confused about which way the improvement should be going, hence the comment about the coordinator restarting. In actual fact, this PR is to avoid remaking sessions on the coordinator, so the coordinator or member restarting is not an issue (in the former case, the coordinator will get a new reusable session and in the latter, the reusable session will persist). As you can see in the image below, the difference from reusing the session is 1.1ms without this change and 0.05ms with the change. |
Prevent this reusable pool session from being destroyed so that it remains valid. This feature can be toggled with the flag reuse-pool-sessions. This commit has been reworked to use Atomic instead of a mutex to prevent a deadlock as shown in CA-400339. Signed-off-by: Steven Woods <steven.woods@citrix.com>
Add a new flag validate-reusable-pool-session to xapi globs which skips the reusable pool session validity check if it is false. This saves time as we are no longer calling pool.get_all for each session. Signed-off-by: Steven Woods <steven.woods@citrix.com>
This can be reverted once we have sufficient confidence from running nightlies Signed-off-by: Steven Woods <steven.woods@cloud.com>
snwoods
force-pushed
the
private/stevenwo/CA-400339
branch
from
1d07b7c to
e42cca1
Compare
|
I've added a commit which defaults reuse_pool_sessions to false, so this PR is a noop and safe to merge. We can then run some nightlies with the commit reverted and then revert the commit on master when we are confident. |
This was first introduced in #5986 but was later reverted as the mutex it introduced caused a deadlock with the DB mutex. This PR has now been reworked to use an Atomic variable, removing the need for the mutex.
This PR allows pool sessions to be reused (if the flag is turned on, which it is by default). This greatly speeds up communications between hosts which is important for repetitive calls e.g. one customer was running get_servertime frequently on all hosts in a pool which was causing problems due to the length of each call.
When we get this reusable session, we can optionally validate the session before using it but this increases the duration of the call, so this is off by default. Excepting a toolstack restart, the reusable session should always be valid as it is excluded from deletion in destroy_db_session